Skip to content

fix(media-uploads): copy media upload files when cloning an activity; freeze modern-format upload paths - #643

Open
caseylocker wants to merge 14 commits into
mainfrom
fix/clone-media-upload-files
Open

caseylocker wants to merge 14 commits into
mainfrom
fix/clone-media-upload-files

Conversation

@caseylocker

@caseylocker caseylocker commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

ref: https://app.clickup.com/t/9014802374/86bchyjfg

Problem

  • SummitService::cloneEvent cloned PresentationMediaUpload records but not their files. The clone's path is built from the clone's presentation ID (always legacy format), so every cloned upload pointed at a path with no file. Seen in prod on OCP Global 2026 (activity 10346 cloned from 10177).
  • A modern-format upload's path is re-derived from the current title slug in getPath(), so renaming an activity (or changing the summit slug) moved the path away from the stored file. Legacy-format paths use only IDs and were unaffected.

Fix

  • Rename: new nullable PresentationMediaUpload.FrozenPathSegment. getPath() uses it for modern-format rows; output is byte-identical for every row where it is null. Version20261014120000 adds the column (Builder), Version20261014120001 backfills the LegacyPathFormat = 0 rows with the segment getPath() produces today (same empty-title rule as generateSlug()).
  • Clone: after cloneEvent commits, it queues one CopyClonedMediaUploadFilesJob per cloned upload (JobDispatcher::withDbFallback). The job is a thin delegate to SummitService::copyClonedMediaUploadFiles (and removeUncopiedClonedMediaUpload from failed()). The logic lives on SummitService rather than PresentationService because it shares a storage lock with processPendingMediaUploads, which lives there. The copy streams each enabled storage leg from the original's path to the clone's (readStream -> saveFromStream, overwriting every attempt), skips a leg the original does not have, and retries (8 tries over about 23 minutes, timeout 1200 s, below retry_after). It copies the filename the clone was queued with, so a later re-upload of the original under a new name does not change what the clone gets.
  • The job never writes to or deletes a clone upload staff have re-uploaded since (filename changed, or any PendingMediaUpload row for it), and retries while a re-upload of the original is still in flight.
  • A per-storage-location Redis lock (ILockManagerService, SummitService::mediaUploadStorageLockName = media_upload_storage.{presentation_id}.{sha1(filename)}.lock) is shared by the job and processPendingMediaUploads. The job holds the lock of both the clone's location and the original's, so a staff upload landing mid-copy is written after the copy, never overwritten by it, and an original is never read while it is being written. The processor skips a locked row untouched and picks it up on the next minute's run. If the lock store itself is unreachable, the processor keeps processing without the lock; a copy waits and retries instead.
  • The job carries both presentation IDs, takes its locks before reading the database, and reads both upload records fresh under them. If staff delete the clone's upload while the copy runs, the copied files are marked deleted.
  • If every attempt fails (including a final-attempt timeout, which Laravel handles in-process), the cleanup removes the clone's upload record before marking its files deleted, so the clone never points at a missing file. Staff then re-upload on the clone.

Tests

  • New: CopyClonedMediaUploadFilesJobTest (26), PresentationMediaUploadPathTest (7), FrozenPathSegmentBackfillMigrationTest (4).
  • Updated: SummitServiceTest (clone queues the copy and the copy lands; dispatch failure; processor skips a locked upload), ProcessPendingMediaUploadsTest (lock dependency; locked row untouched; lock store unavailable; release failure).
  • push.yml: adds the MigrationUnitTests shard and the new root test file to the PresentationMediaUploads shard.
  • Unchanged by this branch and failing identically on main locally: the OAuth2PresentationApiTest + OAuth2SummitEventsApiTest pair (17 failures, 1 error), SponsorUserPermissionTrackingTest (2), and 3 in tests/Unit/Entities.

Deploy notes

  • This changes the pending upload processor.
  • Squash-merge (one intermediate commit has a red unit test).
  • Two migrations run on the image build. Afterwards SELECT COUNT(*) FROM PresentationMediaUpload WHERE LegacyPathFormat = 0 AND FrozenPathSegment IS NULL should be 0. Checked against prod on Nov 13: 221 rows are in scope and the longest title is 113 characters, well inside the 1024-character column.
  • Smoke test on a non-show summit: clone an activity with a deck and confirm the file lands on both storage legs (Spaces and Dropbox). CI covers the Local driver only.
  • Expect LockManagerService ... MAX RETRIES ERROR lines while a clone copy holds a lock the processor wants, or while one copy waits on another (for example a clone of a clone). They are lock misses, not failures: the processor picks the row up on its next run and the copy retries.
  • A copy that cannot proceed retries for about 23 minutes before its cleanup removes the clone's upload. CopyClonedMediaUploadFilesJob::failed in the log is the signal that staff need to re-upload on the clone.
  • If Redis is unreachable, the processor logs lock store unavailable, processing without the storage lock (WARNING) for each row and keeps writing staff uploads. Clone copies do not run without the lock. If the outage outlasts a copy's retries, its cleanup cannot take the lock either: it keeps the clone's upload and logs could not take the storage lock to remove clone media upload (ERROR), and that upload needs a delete or re-upload on the clone.

Known limits

  • A modern-format row whose title changed before the backfill is frozen at the current slug; those rows (2020, summits 8 and 12) were already broken.
  • Between the clone request and the copy finishing, the clone's upload briefly has no file.
  • A clone of a clone made while the first clone's copy is between retries can copy a first clone that is not fully filled yet.
  • A worker killed outright mid-copy (not a timeout) leaves its locks to expire after 1300 s, which delays a same-name upload at either location by up to about 22 minutes.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ed897bea-d57d-4d66-96d5-aac354ed755e
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@github-actions

Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-643/

This page is automatically updated on each push to this PR.

@github-actions

Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-643/

This page is automatically updated on each push to this PR.

@caseylocker
caseylocker marked this pull request as ready for review October 15, 2026 00:47
@caseylocker
caseylocker requested review from smarcet and a balanced review from Copilot October 15, 2026 00:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved concurrency gaps can copy incomplete source files or overwrite and delete staff replacements.

2 open findings
What changed in this PR

Fixes activity cloning to copy media files and preserves modern upload paths across title or summit slug changes.

Changes:

  • Adds queued file copying, shared storage locks, and failure cleanup.
  • Adds and backfills frozen path segments.
  • Expands regression tests and CI coverage.
File Description
tests/​Unit/​Services/​ProcessPendingMediaUploadsTest.php Tests processor lock handling.
tests/​Unit/​Migrations/​FrozenPathSegmentBackfillMigrationTest.php Tests backfill slug rules.
tests/​Unit/​Entities/​PresentationMediaUploadPathTest.php Tests stable upload paths.
tests/​SummitServiceTest.php Tests clone dispatch and processor locking.
tests/​CopyClonedMediaUploadFilesJobTest.php Tests copying, replacement guards, and cleanup.
database/​migrations/​model/​Version20261014120001.php Backfills frozen paths.
database/​migrations/​model/​Version20261014120000.php Adds the frozen-path column.
app/​Services/​Model/​ISummitService.php Defines copy and cleanup contracts.
app/​Services/​Model/​Imp/​SummitService.php Implements copying, locking, and cleanup.
app/​Repositories/​Summit/​DoctrinePendingMediaUploadRepository.php Adds pending-upload existence queries.
app/​Models/​Foundation/​Summit/​Repositories/​IPendingMediaUploadRepository.php Declares pending-upload queries.
app/​Models/​Foundation/​Summit/​PendingMediaUpload.php Centralizes in-flight statuses.
app/​Models/​Foundation/​Summit/​Events/​Presentations/​Presentation.php Extracts reusable slug generation.
app/​Models/​Foundation/​Summit/​Events/​Presentations/​Materials/​PresentationMediaUpload.php Uses frozen modern paths.
app/​Jobs/​CopyClonedMediaUploadFilesJob.php Adds retryable copy job.
.github/​workflows/​push.yml Includes new tests in CI.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

return;
}
// UnacquiredLockException propagates: the processor is writing this location, so retry later
$token = $this->lock_service->acquireLock($lock_name, ISummitService::MEDIA_UPLOAD_STORAGE_LOCK_LIFETIME);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0b9476c. The copy now takes the source location's storage lock as well as the target's, and holds both for the whole copy, so the source's processor (or another copy still filling it, when the source is itself a clone) cannot write the files while they are read. If the source lock cannot be taken, the target lock is released and the job retries.

No lock ordering is needed: acquireLock never blocks (three tries, then UnacquiredLockException), so two copies taking the locks in opposite order cannot deadlock. The job's backoff now adds up to more than the lock lifetime, so a waiting copy outlasts a held lock.

Covered by testHandleThrowsWhenSourceStorageLocked and testHandleReleasesBothLocksAfterCopy.

private function resolveClonedMediaUploadLegs(int $source_media_upload_id, int $target_media_upload_id, string $filename): array
{
return $this->tx_service->transaction(function () use ($source_media_upload_id, $target_media_upload_id, $filename) {
$target = $this->presentation_media_upload_repository->getById($target_media_upload_id);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 0b9476c, without a separate EntityManager.

  • The job payload now carries both presentation IDs, so the lock names are built and the locks taken before anything is read from the database.
  • Under the locks, target and source are read with a new getByIdFresh(), a DQL read with Query::HINT_REFRESH. It does not answer from the identity map and returns null when the row is gone, so a delete then re-add at the same path is seen.
  • The failed() cleanup uses the same read with FOR UPDATE, so it no longer renames a file that a newer upload wrote at that path.

Covered by testHandleLeavesFileAloneWhenTargetDeletedByAnotherProcess and testFailedLeavesFileAloneWhenTargetDeletedByAnotherProcess, which delete the row outside the EntityManager so the stale entity stays managed.

@caseylocker caseylocker self-assigned this Nov 13, 2026
… fresh, keep uploads flowing without the lock store

- hold the source location's storage lock as well as the target's, so a copy
  never reads an original its processor or another copy is still writing
- carry both presentation IDs in the job so the locks are taken before any
  database read, and read both records past the identity map under the locks
- copy from the filename the clone was queued with, not the source's current one
- mark the copied files deleted when the clone's upload is deleted mid-copy
- give the upload processor its own lock lifetime, matched to the stuck-row
  reset, and size the copy job's backoff to outlast a held storage lock
- process pending uploads without the lock when the lock store is unavailable,
  and never let a lock error abort the rest of the batch
@github-actions

Copy link
Copy Markdown

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-643/

This page is automatically updated on each push to this PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants